Greptile Summary

This PR fixes a bug where command records were filtered out during proto deserialization (inside read_batch_from_proto) before the session's limit counters (remaining_count, remaining_bytes) and resume pointer (seq_num) were updated, causing the client to undercount consumed records and bytes — and in the worst case (a batch composed entirely of command records) to skip the seq_num advance entirely, risking duplicate delivery on retry.

The fix correctly defers filtering to after all limit-tracking state is applied, adds is_command_record() to SequencedRecord as the canonical check, and suppresses empty post-filter batches from being yielded to callers.

Confidence Score: 5/5

• Safe to merge; the logic change is correct and consistent across all call sites.
• All findings are P2 (missing tests). The core fix — moving filtering after limit tracking — is mechanically straightforward and correctly applied in both the session and unary read paths. No blocking issues remain.
• No files require special attention beyond adding test coverage for the ignore_command_records path.

Important Files Changed

Sequence Diagram

sequenceDiagram
    participant Caller
    participant run_read_session
    participant Server

    Server-->>run_read_session: ReadBatch (data + command records)
    Note over run_read_session: read_batch_from_proto() — full unfiltered batch
    run_read_session->>run_read_session: Update seq_num (last unfiltered record)
    run_read_session->>run_read_session: Decrement remaining_count / remaining_bytes (full batch)
    alt "ignore_command_records=True"
        run_read_session->>run_read_session: Filter out command records
        alt filtered batch has records
            run_read_session-->>Caller: yield ReadBatch (data records only)
        else all records were command records
            Note over run_read_session: batch dropped silently — limits already tracked
        end
    else "ignore_command_records=False"
        run_read_session-->>Caller: yield ReadBatch (all records)
    end

This is a comment left during a code review.
Path: src/s2_sdk/_s2s/_read_session.py
Line: 106-115

Comment:
**No tests cover `ignore_command_records` filtering**

There are no tests exercising the `ignore_command_records=True` path in `run_read_session` — neither the limit-tracking fix (the core bug) nor the suppression of empty post-filter batches. A regression here would be silent. Consider adding at least one unit/integration test that verifies a session with a mix of command and data records correctly decrements `remaining_count`/`remaining_bytes` against the full (unfiltered) record count and only yields batches containing data records.

How can I resolve this? If you propose a fix, please make it concise.

_{Reviews (1): Last reviewed commit: "initial commit" | Re-trigger Greptile}